Skip to content

Attributes conversion to metadata#583

Merged
BryonLewis merged 11 commits into
mainfrom
client/attributes-update
Mar 3, 2021
Merged

Attributes conversion to metadata#583
BryonLewis merged 11 commits into
mainfrom
client/attributes-update

Conversation

@BryonLewis
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis commented Feb 18, 2021

  • Creates a new useAttributes which provides the attributes from the metaData and also allows for the deletion and updating of attributes.
  • the provides has implemented a useAttributes for an array of attributes (Data structure that TrackDetails.vue expects)
  • added setAttribute and deleteAttribute methods to the handler
  • These updates get tied into an updated markPendingChanges which can accept Track and Attribute now
  • Saving attributes call a new PUT /attributes endpoint which modifies the metadata in the view version
  • Deprecated older /attribute endpoints. Do we want to remove them now?
  • Desktop version has a dataset/id/attributes endpoint which it can use to save attributes data to the meta.json
  • Removed the Settings from the toolbar, but left the Vue Component if we want to use in the future
  • Placed TODOs for editing track values when attributes are changed. I.E. if you delete an attribute should it be deleted from all tracks?

@BryonLewis BryonLewis force-pushed the client/attributes-update branch from cbaa4ae to 34530d9 Compare February 18, 2021 16:43
Copy link
Copy Markdown
Collaborator Author

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some items I should address.

Comment thread client/dive-common/components/AttributesSubsection.vue Outdated
Comment thread client/dive-common/components/TrackDetailsPanel.vue Outdated
Comment thread client/dive-common/use/useSave.ts Outdated
Comment thread client/dive-common/use/useSave.ts Outdated
Comment on lines +61 to +78
} else if (type === 'track' && data !== undefined && (data as Track).trackId !== undefined) {
const track = data as Track;
if (action === 'delete') {
pendingChangeMap.delete.add(track.trackId);
pendingChangeMap.upsert.delete(track.trackId);
} else if (action === 'upsert') {
pendingChangeMap.delete.delete(track.trackId);
pendingChangeMap.upsert.set(track.trackId, track);
}
} else if (type === 'attribute' && data !== undefined && (data as Attribute)._id !== undefined) {
const attribute = data as Attribute;
if (action === 'delete') {
pendingChangeMap.attributeDelete.add(attribute._id);
pendingChangeMap.attributeUpsert.delete(attribute._id);
} else if (action === 'upsert') {
pendingChangeMap.attributeDelete.delete(attribute._id);
pendingChangeMap.attributeUpsert.set(attribute._id, attribute);
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably a way I can make this more compact and simpler

Comment thread client/platform/desktop/backend/server.ts Outdated
@BryonLewis BryonLewis force-pushed the client/attributes-update branch from 1db6f85 to 9ab1ea8 Compare February 24, 2021 21:01
@BryonLewis BryonLewis marked this pull request as ready for review February 24, 2021 21:11
@BryonLewis BryonLewis changed the title [WIP] - Attributes conversion to metadata Attributes conversion to metadata Feb 24, 2021
@BryonLewis BryonLewis requested a review from subdavis February 26, 2021 19:13
@subdavis
Copy link
Copy Markdown
Contributor

subdavis commented Mar 1, 2021

This is really good. I'm still reviewing it, but I'm pretty excited about getting this merged.

subdavis and others added 3 commits March 2, 2021 08:36
* Try type gurads

* markChangesPending update (#609)

Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple renames, one larger question about _id

Comment thread client/src/use/useAttributes.ts Outdated
attributes.value = metadataAttributes;
}

const getAttributes = computed(() => Object.values(attributes.value));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAttributes sounds like a function in our typical parlance.

Can we call it attributeList or something that sounds like an object?

Comment thread server/dive_server/viame.py Outdated
Description("")
.modelParam("id", model=Attribute, required=True)
.jsonParam("data", "", requireObject=True, paramType="body")
.deprecated()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the client isn't using these, can we just remove them?

Copy link
Copy Markdown
Contributor

@subdavis subdavis Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see they're part of the old UI that was commented out. I actually see no reason to keep this or any of the old code, especially since the JS functions that call these methods are gone, so they're totally dead code now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure I didn't know if we wanted to leave them up for any reason.

Comment thread server/dive_server/viame.py Outdated
for attribute_id in delete:
attributes_dict.pop(str(attribute_id), None)
for attribute in upsert:
attributes_dict[str(attribute['_id'])] = attribute
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to have some server-side validation here.

When saving a track, we hydrate a Pydantic Track with the json object. Normally this stuff would be enforced by a relational DB, but we don't have that, so Pydantic is the best we can really do.

See viame_detection.save_detection for an example of what I'm talking about. Should be a small change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I wonder if we should use a key signifier other than _id. That's a girder convention, and attribute is actually a girder model, but we aren't using it anymore. It confused me that there was still an _id.

What do you think about just id since this isn't a girder model and doesn't refer to one?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we even need an id anymore. The tracks attributes and detection attributes are limited by their name (because of the CSV file). So really the identifier is a combination of the belongs and the name.

Comment thread client/src/use/useAttributes.ts Outdated
let oldAttribute;
if (data._id === '') {
// eslint-disable-next-line no-param-reassign
data._id = `${data.belongs}_${data.name}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes no-param-reassign is a nuisance, but here I think it's protecting you. This function has an undocumented side-effect in that it sets the _id of the passed attribute. The caller relies on this behavior in an unclear way.

I actually see _id as unnecessary. It's synthesized from other properties and doesn't really provide any information. You could get rid of _id completely, have the map key be an implementation detail of useAttributes, and change the signature of deleteAttirbute to also accept the full object rather than an id.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to change the setAttribute and deleteAttribute to accept the full object. I started removing the _id from all Attributes and it just felt really wrong after a while computing the index each time I wanted to access it. So I think I'll rename the _id to key and prevent any reassignment.

I think it might be better to have setAttribute be able to accept an oldAttribute if one exists (for the instance of renaming the attribute and changing the index). So if a rename occurs it will delete the oldAttribute and replace it with the new one with the updated index. There are already client side checks to make sure you don't create a new attribute with the same key as existing ones.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with key, but that leaves the side-effect issue. An option is to have setAttribute return a clone with key set such that using it requires localReactiveCopy.value = setAttribute(localReactiveCopy.value);?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data Attribute isn't a direct reference, it's a new Attribute item built up of the changes and the oldAttribute is a reference to the item in the list and is only included if the user is updating an existing attribute.
The oldAttribute can be updated in the process by the name change causing a delete and refresh or by changing the type. When this occurs the item that had a reference to that as being open is already closed (AttributeEditor.vue) and as as the VueSet updates the attributes list it is then updated in the trackDetails.vue. The value isn't returned and instead relies on the reactivity of the attributes list provided to trackDetails.vue (Not ideal, but who is going to have an extremely large list on one dataset).

image

Please let me know if I'm missing something here and seeing stuff incorrectly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about the invocation from TrackDetailsPanel.vue, line 130.

      try {
        // before: saveData._id === ''
       await setAttribute({ data: saveData.data });
        // after: saveData._id === 'Belongs_NewName';
      } catch (err) {
        editingError.value = err.message;
      }

That _id change on saveData is a side-effect I'd like to avoid. It looks like you may have already changed this, so I'll review once you push.

@BryonLewis BryonLewis force-pushed the client/attributes-update branch from ad68c3e to 014cb7c Compare March 2, 2021 23:04
@subdavis subdavis self-requested a review March 3, 2021 16:33
Copy link
Copy Markdown
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🇮🇹

@BryonLewis BryonLewis merged commit 8eba631 into main Mar 3, 2021
@BryonLewis BryonLewis deleted the client/attributes-update branch March 3, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants